Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified event-based communicator interface #106

Merged
merged 7 commits into from
May 27, 2016

Conversation

jovanbulck
Copy link
Collaborator

@jovanbulck jovanbulck commented Apr 20, 2016

Improved cohesion: worker catches browser exceptions and indicates
the kind of error to the communicator, which is the sole responsible
to deliver the appropriate error message to the end user and restore
the screen/cursor.

Also kicked out much of the logout functionality for readability
purposes. We'll re-introduced logout to the code base after the
beta milestone.

Improved cohesion: worker catches browser exceptions and indicates
the kind of error to the communicator, which is the sole responsible
to deliver the appropriate error message to the end user and restore
the screen/cursor.

Worker indicates the error via a code (int) for now, but we might
consider using a dedicated ErrorReport object that includes more
information, depending on the type of error (eg rccode, inst, IP
address, html dump, stacktrace, ...)

Also kicked out much of the logout functionality for readability
purposes. We'll re-introduced logout to the code base after the
beta milestone.
self.print_fail()
## fail gracefully
self.finalize_session(False)
## provide the user with an appropriate error message
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually smells very bad. I'm working on refactoring this to use explicit communicator methods for each kind of failure

This refactors the single parameterized method into a set of _explicit_
methods for each kind of expected failure, with additional info passed
as parameters to the communicator. Likewise, _unexpected_ failure (i.e.
exceptions) are handled via a single 'eventFailureInternalError'
communicator method that takes the traceback as an argument.

This nicely decouples visualisation concerns from worker functionality,
and ensures all kinds of failures are handled gracefully.
Use the Python atexit module to ensure the eventExit() method of
the running communicator is always called. This nicely allows a
communicator to for example redraw the cursor (cf issue #9).

Also improved the readibility of the browser URL requests, and added
a timeout for all remote server requests.
@jovanbulck jovanbulck mentioned this pull request Apr 24, 2016
7 tasks
@jovanbulck jovanbulck force-pushed the dev-communicator-exceptions branch 2 times, most recently from 9fd8a6d to 0bf2073 Compare April 24, 2016 21:14
@jovanbulck
Copy link
Collaborator Author

jovanbulck commented Apr 24, 2016

@GijsTimmers have a look at the changes: I think this can be merged. Also fixed some code quality checks and Travis CI script

The lazy importing mechanism only imports a communicator when it has been
requested by the front-end, via the fabriek. This ensures an end-user
does not have to install all the dependencies for all the communicators
in order to run kotnetcli. The front-end is thus responsible to catch
any ImportErrors thrown at communicator creation time.

This nicely maintains the behavior that *only* the front-end and the
communicators encapsulate user interaction + only the front-end or
worker may terminate (sys.exit) the program.

This fixes issue #9 .
@jovanbulck jovanbulck mentioned this pull request May 25, 2016
@GijsTimmers
Copy link
Owner

Looking good, thanks for the hard work.

@GijsTimmers GijsTimmers merged commit 625ae6e into dev May 27, 2016
@GijsTimmers GijsTimmers deleted the dev-communicator-exceptions branch May 27, 2016 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants